-
-
Notifications
You must be signed in to change notification settings - Fork 128
change(web): move KeyboardProcessor.Codes into common/web/types 🏗️ #11913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
6fecc73
to
1798b7a
Compare
68a1330
to
a1bdce0
Compare
1798b7a
to
f5d4587
Compare
a1bdce0
to
2d9d847
Compare
f5d4587
to
f17f847
Compare
03d8994
to
bd732fa
Compare
Also add some trivial unit tests for `Codes` to satisfy coverage threshold. Fixes: #8146
bd732fa
to
3fcb6c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this refactor PR. I am going to requesting a significant change here.
I would like to be careful about what we move into common/types. I am already trying to extricate myself from Developer-specific files that I put there, so don't want to do the same thing for KeymanWeb-specific content.
The constants in codes.ts do belong in common/types -- but they are nearly all already there, in kmx.ts:
keyman/common/web/types/src/kmx/kmx.ts
Lines 346 to 364 in 3497e97
public static readonly LCTRLFLAG = 0x0001; // Left Control flag | |
public static readonly RCTRLFLAG = 0x0002; // Right Control flag | |
public static readonly LALTFLAG = 0x0004; // Left Alt flag | |
public static readonly RALTFLAG = 0x0008; // Right Alt flag | |
public static readonly K_SHIFTFLAG = 0x0010; // Either shift flag | |
public static readonly K_CTRLFLAG = 0x0020; // Either ctrl flag | |
public static readonly K_ALTFLAG = 0x0040; // Either alt flag | |
//public static readonly K_METAFLAG = 0x0080; // Either Meta-key flag (tentative). Not usable in keyboard rules; | |
// Used internally (currently, only by KMW) to ensure Meta-key | |
// shortcuts safely bypass rules | |
// Meta key = Command key on macOS, Windows key on Windows | |
public static readonly CAPITALFLAG = 0x0100; // Caps lock on | |
public static readonly NOTCAPITALFLAG = 0x0200; // Caps lock NOT on | |
public static readonly NUMLOCKFLAG = 0x0400; // Num lock on | |
public static readonly NOTNUMLOCKFLAG = 0x0800; // Num lock NOT on | |
public static readonly SCROLLFLAG = 0x1000; // Scroll lock on | |
public static readonly NOTSCROLLFLAG = 0x2000; // Scroll lock NOT on | |
public static readonly ISVIRTUALKEY = 0x4000; // It is a Virtual Key Sequence | |
public static readonly VIRTUALCHARKEY = 0x8000; // Keyman 6.0: Virtual Key Cap Sequence NOT YET |
and virtual-key-constants.ts:
keyman/common/web/types/src/consts/virtual-key-constants.ts
Lines 5 to 143 in 2b2661b
/** | |
* May include non-US virtual key codes | |
*/ | |
export const USVirtualKeyCodes = { | |
K_BKSP:8, | |
K_TAB:9, | |
K_ENTER:13, | |
K_SHIFT:16, | |
K_CONTROL:17, | |
K_ALT:18, | |
K_PAUSE:19, | |
K_CAPS:20, | |
K_ESC:27, | |
K_SPACE:32, | |
K_PGUP:33, | |
K_PGDN:34, | |
K_END:35, | |
K_HOME:36, | |
K_LEFT:37, | |
K_UP:38, | |
K_RIGHT:39, | |
K_DOWN:40, | |
K_SEL:41, | |
K_PRINT:42, | |
K_EXEC:43, | |
K_INS:45, | |
K_DEL:46, | |
K_HELP:47, | |
K_0:48, | |
K_1:49, | |
K_2:50, | |
K_3:51, | |
K_4:52, | |
K_5:53, | |
K_6:54, | |
K_7:55, | |
K_8:56, | |
K_9:57, | |
K_A:65, | |
K_B:66, | |
K_C:67, | |
K_D:68, | |
K_E:69, | |
K_F:70, | |
K_G:71, | |
K_H:72, | |
K_I:73, | |
K_J:74, | |
K_K:75, | |
K_L:76, | |
K_M:77, | |
K_N:78, | |
K_O:79, | |
K_P:80, | |
K_Q:81, | |
K_R:82, | |
K_S:83, | |
K_T:84, | |
K_U:85, | |
K_V:86, | |
K_W:87, | |
K_X:88, | |
K_Y:89, | |
K_Z:90, | |
K_NP0:96, | |
K_NP1:97, | |
K_NP2:98, | |
K_NP3:99, | |
K_NP4:100, | |
K_NP5:101, | |
K_NP6:102, | |
K_NP7:103, | |
K_NP8:104, | |
K_NP9:105, | |
K_NPSTAR:106, | |
K_NPPLUS:107, | |
K_SEPARATOR:108, | |
K_NPMINUS:109, | |
K_NPDOT:110, | |
K_NPSLASH:111, | |
K_F1:112, | |
K_F2:113, | |
K_F3:114, | |
K_F4:115, | |
K_F5:116, | |
K_F6:117, | |
K_F7:118, | |
K_F8:119, | |
K_F9:120, | |
K_F10:121, | |
K_F11:122, | |
K_F12:123, | |
K_NUMLOCK:144, | |
K_SCROLL:145, | |
K_LSHIFT:160, | |
K_RSHIFT:161, | |
K_LCONTROL:162, | |
K_RCONTROL:163, | |
K_LALT:164, | |
K_RALT:165, | |
K_COLON:186, | |
K_EQUAL:187, | |
K_COMMA:188, | |
K_HYPHEN:189, | |
K_PERIOD:190, | |
K_SLASH:191, | |
K_BKQUOTE:192, | |
K_LBRKT:219, | |
/** | |
* == K_OEM_5, 0xDC | |
*/ | |
K_BKSLASH:220, | |
K_RBRKT:221, | |
K_QUOTE:222, | |
/** | |
* ISO B00, key to right of left shift, not on US keyboard, | |
* 0xE2, K_OEM_102 | |
*/ | |
K_oE2:226, | |
K_OE2:226, | |
K_oC1:193, // ISO B11, ABNT-2 key to left of right shift, not on US keyboard | |
K_OC1:193, | |
'K_?C1':193, | |
'k_?C1':193, | |
K_oDF:0xDF, | |
K_ODF:0xDF, | |
/*K_LOPT:50001, | |
K_ROPT:50002, | |
K_NUMERALS:50003, | |
K_SYMBOLS:50004, | |
K_CURRENCIES:50005, | |
K_UPPER:50006, | |
K_LOWER:50007, | |
K_ALPHA:50008, | |
K_SHIFTED:50009, | |
K_ALTGR:50010, | |
K_TABBACK:50011, | |
K_TABFWD:50012*/ | |
}; |
Given this, I am not sure that this move makes sense -- we don't solve the WETness of the constants. So here's an alternate proposal:
- consider a reorg of kmx.ts to move modifier constants into consts/modifier-key-constants.ts (and re-export them from kmx.ts to maintain 100% API surface consistency)
- keep codes.ts in keyboard-processor
- harmonize any inconsistencies between the constants in codes.ts and modifier-key-constants.ts / virtual-key-constants.ts. This may mean adding some constants to those two files (but without changing the pattern in which they are declared.)
- finally, remove the constant declarations from codes.ts, and instead import and re-export the ones found in modifier-key-constants.ts / virtual-key-constants.ts. If necessary, add wrappers so that we can match the way kmw currently uses the constants. (Avoid rewriting other aspects of kmw at this point)
I think this would make the change quite a lot smaller, as most of the references in kmw wouldn't change, just codes.ts. It would also address the WETness, which IMO is the primary driver for this refactor.
I appreciate that this is a big change request -- but really feel like it will result in a better outcome.
Note: some of my comments added in this review probably become irrelevant in light of this more fundamental change request
var modifier=0; | ||
let modifier=0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason these were changed in this PR? I would prefer not to have even minimal changes like this in a refactor PR
// We should start splitting off code needed by keyboards even without a KeyboardProcessor active. | ||
|
||
// see also: common/web/types/src/kmx/kmx.ts | ||
|
||
const Codes = { | ||
export const Codes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a overly broad export name! Can we rename it? KeyCodes
?
].forEach(keyId => assert.isTrue(Codes.isFrameKey(keyId), `isFrameKey(${keyId})`)); | ||
|
||
[ | ||
'K_ESC', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a frame key, shouldn't it? But a job for a future PR if so.
'K_ALT', // not currently included in isFrameKey | ||
'K_CTRL', // not currently included in isFrameKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These also should be frame keys, shouldn't they? But also a job for a future PR if so.
}); | ||
|
||
describe('test getModifierState()', function () { | ||
it('should properly identify modifiers from layer id', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think many of these layerIds are deprecated, and it would be good to split tests for deprecated layerIds out.
@@ -1,4 +1,4 @@ | |||
export { Codes, DeviceSpec, Keyboard, KeyboardProperties, SpacebarText } from '@keymanapp/keyboard-processor'; | |||
export { DeviceSpec, Keyboard, KeyboardProperties, SpacebarText } from '@keymanapp/keyboard-processor'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Codes
be re-exported, given it was exported here before? I don't know how "public" these exports are... @jahorton? (And if we rename Codes
then perhaps its name should be maintained here export { KeyCodes as Codes } from '@keymanapp/common-types';
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that we only even modularized to this extent with Keyman 17, I severely doubt anyone external is relying upon Codes
here. If everything is building fine, we're good.
I would be a touch worried about Web's test-sequence "recording" module... but that's built via ./web/build.sh tools
, which is triggered during standard Web CI builds. As even that didn't fall over, in the absolute worst case, all that would be needed is to tidy up the relevant pages.
tl;dr: nah, no need to re-export.
@@ -115,13 +114,13 @@ const Codes = { | |||
* @return {number} modifier key state (desktop keyboards) | |||
*/ | |||
getModifierState(layerId: string): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of the functions in this file should be moved over -- just the constants. These functions should stay in KeymanWeb's source, because they are not generic nor consistent enough to be put into common/types.
@@ -1,9 +1,8 @@ | |||
// TODO: Move to separate folder: 'codes' | |||
// We should start splitting off code needed by keyboards even without a KeyboardProcessor active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be deleted; and we need a header.
// We should start splitting off code needed by keyboards even without a KeyboardProcessor active. | |
/* | |
* Keyman is copyright (C) SIL International. MIT License. | |
* | |
* Keyboard key codes and modifier bitmasks. | |
*/ |
export { default as Codes } from "./text/codes.js"; | ||
export * from "./text/codes.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not great (double-exporting and flattening of exports like this) and I am glad it's gone ... but do we need to maintain the exports for compatibility? @jahorton
Also exported in web/src/engine/osk/src/index.ts (why?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, if it builds, I assume it's fine. Standard Web CI builds even build the development tools, which would be the only thing I'd worry about falling over. (They'd fail the build if they fell over.)
Why would we not just rework the definition patterns to be consistent for each Codes property? The excerpted
Granted, some of the constant names differ from what's in I do understand we'd probably need a wrapper to "bundle" all the codes into their "final form" - as properties of Web's current |
I would like to avoid a cascade of changes into Developer at this point -- there are many changes happening in Developer at the same time. That block is a match for kmx_file.h, also. But we could certainly put an object wrapper into common/types (that is then imported into Codes.ts). Then at least the wrapper and the original definitions are in the same place. |
So in our discussion, we said that we'd like to keep the
syntax because that's used in Developer. However,
is also used in Developer (via
Is this still true? Then we will need both? |
Yes, we'll need both. Keyboards build with Debug configuration have references to https://gist.github.com/mcdurdin/1aeedf74c2127165bc63f705d41b3292#file-khmer_angkor-js-L9-L10 var modCodes = keyman.osk.modifierCodes;
var keyCodes = keyman.osk.keyCodes; |
Superseded by #12072 |
Fixes: #8146
@keymanapp-test-bot skip